Conversation
emily-vanark
left a comment
emily-vanark
left a comment
There was a problem hiding this comment.
I left a few comments... Mostly I'm wondering if / how the retry logic can be tested.
|
@emily-vanark thanks for your review! Yes tests were missing. They're added at test_llm_interface.py 🚀 |
|
|
||
| def __init__(self, name: str, system_prompt: Optional[str] = None): | ||
| def __init__( | ||
| self, name: str, system_prompt: Optional[str] = None, max_retries: int = 3 |
There was a problem hiding this comment.
until now I have tried to avoid (not always successfully) putting defaults outside of the main entry point, as it might introduce subtle bugs.
while it makes the script code a little longer, i wonder if putting an optional arg there and passing it down makes it more coherent with the existing codebase?
There was a problem hiding this comment.
of course, this opens a new question: all the functions are getting bloated, and maybe an config file/arg is now needed
There was a problem hiding this comment.
Thanks for this!
yeah, I don't like the bloat! I noticed langchain also supports max_retries, but that's only for langchain-supported endpoints.
I'll revisit this after tending to the other bloat which are the other open PRs 😅
3 participants
Description
Simulated conversations continued even if an error occured due to
generate_responsereturning an error as a string:chatbot: Error generating response: Error code: 400 - {'type': 'error', 'error': {'type': 'invalid_request_error', 'message': 'messages.2: all messages must have non-empty content except for the optional final assistant message'}, 'request_id': 'req_011CX84UXrNZGnUz2i9YM7AX'}This PR implements a retry function for specific errors, like rate limit responses (429) and the like, and only breaks the app if max retries is reached.
Issue
Resolves SAF-163